Skip to content

Repository navigation

Abort SSL connections on close, rather than waiting for remote EOF. - #167

Merged
lovelydinosaur merged 14 commits into
masterfrom
abort-ssl-connections-on-close
Nov 25, 2020
Merged

Abort SSL connections on close, rather than waiting for remote EOF.#167
lovelydinosaur merged 14 commits into
masterfrom
abort-ssl-connections-on-close

Conversation

@lovelydinosaur

@lovelydinosaur lovelydinosaur commented Aug 20, 2020

Copy link
Copy Markdown
Member

In order to close encode/httpx#825 and encode/httpx#914

Probably needs tests to check that:

  • We have ResourceWarnings prior to this, but don't have any after it.
  • Possibly an integration test making a request to a poor SSL shutdown URL, to show that it doesn't hang on close.

@lovelydinosaur lovelydinosaur added the bug Something isn't working label Aug 25, 2020
@florimondmanca

florimondmanca commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

Possibly an integration test making a request to a poor SSL shutdown URL, to show that it doesn't hang on close.

I looked at https://badssl.com to see if they had a public endpoint for this scenario, but it doesn't seem so (only mostly scenarios for certs-related issues).

@agronholm

agronholm commented Sep 20, 2020

Copy link
Copy Markdown
Contributor

All AnyIO backends should do this properly when standard_compatible=False is passed to TLSStream.wrap() (as backends/anyio.py does).

@lovelydinosaur

lovelydinosaur commented Oct 6, 2020

Copy link
Copy Markdown
Member Author

@agronholm Grand - good to know. Any pointers on good ways to test our behaviour before/after this fix?

@agronholm

agronholm commented Oct 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

If the client SSLSocket is receiving an unexpected EOF, it should raise ssl.SSLEOFError. Does the http server you test against unconditionally abort the connection on close?

cdeler
cdeler approved these changes Nov 16, 2020
View reviewed changes

@cdeler cdeler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we really need to do that.

As an alternative we can introduce a "close" timeout as the "connect" timeout we already have which can be fired as a last resort

florimondmanca
florimondmanca approved these changes Nov 16, 2020
View reviewed changes

@florimondmanca florimondmanca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Ideally we'd be adding some tests as Tom suggested in PR description, but I'm also happy enough to merge and release and see if anything breaks. :-)

@cdeler

cdeler commented Nov 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

I think, I hit this problem (probably it was exactly this unclosed socket) while I tried to write the test for #235

@lovelydinosaur lovelydinosaur mentioned this pull request Nov 23, 2020
Closed
1 of 2 tasks
@lovelydinosaur
lovelydinosaur merged commit 6abf663 into master Nov 25, 2020
@lovelydinosaur
lovelydinosaur deleted the abort-ssl-connections-on-close branch November 25, 2020 15:38
This was referenced Dec 7, 2020
Merged
Closed
@lazka

lazka commented Jan 30, 2021

Copy link
Copy Markdown

I'm hitting some problems potentially caused by this change: encode/httpx#1459

Any ideas?

@florimondmanca florimondmanca mentioned this pull request Feb 4, 2021
Closed
@florimondmanca florimondmanca mentioned this pull request Feb 12, 2021
Closed
This was referenced May 12, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@florimondmanca florimondmanca florimondmanca approved these changes
@cdeler cdeler cdeler approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ResourceWarning: unclosed transport <_SelectorSocketTransport fd=6> (asyncio only)

5 participants

@lovelydinosaur @florimondmanca @agronholm @cdeler @lazka